Skip to content

Fix #76675 - Leave a reference to the resource in the curl handle #3744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix #76675 - Leave a reference to the resource in the curl handle #3744

wants to merge 1 commit into from

Conversation

pmmaga
Copy link
Contributor

@pmmaga pmmaga commented Jan 16, 2019

Link for bugsnet: https://bugs.php.net/bug.php?id=76675

curl_write and curl_write_header expect the curl handle to have the reference to their resource set. Currently, the handles returned by _php_server_push_callback miss them.

@nikic
Copy link
Member

nikic commented Jan 16, 2019

I'm getting a segfault when running the added test case:

#0  Curl_init_do (data=data@entry=0x555556b52920, conn=conn@entry=0x0) at url.c:4702
#1  0x00007ffff700c50a in Curl_multi_add_perform (multi=<optimized out>, 
    data=data@entry=0x555556b52920, conn=conn@entry=0x555556a71950) at multi.c:1130
#2  0x00007ffff7025268 in push_promise (frame=0x555556b45518, conn=0x555556a71950, 
    data=0x555556a68260) at http2.c:486
#3  on_frame_recv (session=0x555556b45380, frame=0x555556b45518, userp=0x555556a71950)
    at http2.c:621
#4  0x00007ffff5f85de9 in nghttp2_session_mem_recv ()
   from /usr/lib/x86_64-linux-gnu/libnghttp2.so.14
#5  0x00007ffff7026086 in http2_recv (conn=0x555556a71950, sockindex=<optimized out>, 
    mem=0x555556a6d550 "", len=<optimized out>, err=0x7fffffff9f34) at http2.c:1581
#6  0x00007ffff6ff038d in Curl_read (conn=conn@entry=0x555556a71950, sockfd=3, 
    buf=0x555556a6d550 "", sizerequested=16384, n=n@entry=0x7fffffff9fe0) at sendf.c:741
#7  0x00007ffff7002669 in readwrite_data (comeback=0x7fffffffa06b, done=0x7fffffffa069, 
    didwhat=<synthetic pointer>, k=0x555556a68320, conn=0x555556a71950, data=0x555556a68260)
    at transfer.c:475
#8  Curl_readwrite (conn=0x555556a71950, data=data@entry=0x555556a68260, 
    done=done@entry=0x7fffffffa069, comeback=comeback@entry=0x7fffffffa06b) at transfer.c:1118
#9  0x00007ffff700ce9b in multi_runsingle (multi=multi@entry=0x555556a56a70, now=..., 
    data=data@entry=0x555556a68260) at multi.c:1871
#10 0x00007ffff700e3f4 in curl_multi_perform (multi=0x555556a56a70, running_handles=0x7fffffffa230)
    at multi.c:2136
#11 0x00005555558d8595 in zif_curl_multi_exec (execute_data=0x7fffeea1e2e0, 
    return_value=0x7fffeea1e200) at /home/nikic/php-7.2/ext/curl/multi.c:297

@petk petk added the Bug label Jan 16, 2019
@pmmaga
Copy link
Contributor Author

pmmaga commented Jan 16, 2019

I'm not being able to reproduce the segfault. Can you tell me the versions of curl and nghttp2 you have installed?

@nikic
Copy link
Member

nikic commented Jan 17, 2019

@pmmaga This is my curl --version output:

curl 7.58.0 (x86_64-pc-linux-gnu) libcurl/7.58.0 OpenSSL/1.1.0g zlib/1.2.11 libidn2/2.0.4 libpsl/0.19.1 (+libidn2/2.0.4) nghttp2/1.30.0 librtmp/2.3
Release-Date: 2018-01-24
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IDN IPv6 Largefile GSS-API Kerberos SPNEGO NTLM NTLM_WB SSL libz TLS-SRP HTTP2 UnixSockets HTTPS-proxy PSL

@pmmaga
Copy link
Contributor Author

pmmaga commented Jan 17, 2019

@nikic The segfault seems caused by curl itself, upgrading it to >=7.61 should fix the issue.

@nikic
Copy link
Member

nikic commented Jan 18, 2019

Merged as 32ae716. I've added a SKIPIF for curl < 7.61.

@nikic nikic closed this Jan 18, 2019
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 18, 2019
Merged
@nicolas-grekas
Copy link
Contributor

Thank you @pmmaga and @nikic!

@Nyholm
Copy link
Contributor

Nyholm commented Jan 20, 2019

Hm.. I created a docker image and complied PHP to test this PR. However, I failed to make it work... In fact, I failed to make any curl requests..

When I base my image on alpine:3.4 I get curl version 7.60.0 and I can make normal curl requests as expected.

When I base my image on apline:3.6 I get curl version 7.61.1. I cannot reproduce the output of the test in this PR nor can I make any normal curl request...

➜  docker run -it --rm --name foo -v "$PWD":/usr/src/myapp -w /usr/src/myapp php-latest-34 php test3744.php   
Curl Version: 474112
Curl Version: 7.60.0

➜  docker run -it --rm --name foo -v "$PWD":/usr/src/myapp -w /usr/src/myapp php-latest-34 php simple-curl.php 

Output: 56507%                                                                                                                                                                               

➜  docker run -it --rm --name foo -v "$PWD":/usr/src/myapp -w /usr/src/myapp php-latest-36 php test3744.php   
Curl Version: 474369
Curl Version: 7.61.1

➜  docker run -it --rm --name foo -v "$PWD":/usr/src/myapp -w /usr/src/myapp php-latest-36 php simple-curl.php

See https://github.com/Nyholm/php-test-server-push

It is likely that I'm doing something very wrong, but I just wanted to report my findings (or incompetence) =)

@pmmaga
Copy link
Contributor Author

pmmaga commented Jan 20, 2019

Hi @Nyholm. For curl to support http2 it needs to be built with libnghttp2. This seems to be the case on alpine >=3.8. Based on the dockerfile of your repo, switching the base image to alpine:3.8 (and replacing openssl-dev with libressl-dev due to conflicts), I managed to run the 3 examples successfully.

@Nyholm
Copy link
Contributor

Nyholm commented Jan 21, 2019

Ah, Okey, thank you.

How can I, from a PHP library, figure out if the current runtime has http2 support or not? I know I can check PHP and curl version but how do I check if it is built with libnghttp2?

@nicolas-grekas
Copy link
Contributor

Would that do it?: if (CURL_VERSION_HTTP2 & curl_version()['features'])

@Nyholm Nyholm mentioned this pull request Jan 22, 2019
Merged
@nicolas-grekas
Copy link
Contributor

Actually, there is another bug here: if I add $callback = null; just after curl_multi_setopt($mh, CURLMOPT_PUSHFUNCTION, $callback); in the test file, things go bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pmmaga @nikic @nicolas-grekas @Nyholm @petk